Refine bash completion option handling and context awareness#3773
Refine bash completion option handling and context awareness#3773PeterDaveHello wants to merge 1 commit intonvm-sh:masterfrom
Conversation
Make option suggestions context-aware by subcommand and argument position to avoid invalid combinations. Avoid stale or duplicate suggestions (including value flags and glob-like aliases) while preserving current-word completion. Align option lists with nvm --help and observed CLI behavior for run/exec/ls/uninstall.
|
Related Documentation No published documentation to review for changes on this repository. Write your first living document How did I do? Any feedback? |
This comment was marked as spam.
This comment was marked as spam.
| -*) __nvm_options; return 0 ;; | ||
| esac | ||
|
|
||
| for (( i=1; i < COMP_CWORD; i++ )); do |
There was a problem hiding this comment.
The subcommand/argument parsing loop here (lines 247-281) is nearly identical to the one in __nvm_options() (lines 112-138). This is ~60 lines of duplicated logic that will need to be updated in two places whenever the parsing rules change.
Please extract this into a shared helper function that sets shell variables (e.g., subcommand, has_arg, arg_count, skip_next, etc.) and call it from both __nvm_options() and __nvm().
| if [ "${found_subcommand}" -eq 1 ] && [ "${has_arg}" -eq 1 ] && [ "${allow_after_arg}" -ne 1 ] && [ "${install_post_arg}" -ne 1 ]; then | ||
| OPTIONS='' | ||
| else | ||
| case "${subcommand}" in |
There was a problem hiding this comment.
These per-subcommand option lists will silently become stale whenever nvm adds or removes flags. We need a test that validates that bash_completion is updated when nvm's arg list changes - eg, a test that parses the case statement in nvm() for each subcommand's accepted flags and compares them against the options listed here, or better, the help output.
| noglob_set=0 | ||
| case "$-" in | ||
| *f*) noglob_set=1 ;; | ||
| esac |
There was a problem hiding this comment.
This O(n×m) filtering (all options × all COMP_WORDS) is unnecessary - compgen -W already handles prefix matching and deduplication against the current word. The only case this helps is suppressing options that were already used earlier in the command line, but that's not typically how bash completions work (most tools allow repeating flags). If this behavior is desired, it should be documented as intentional, but I'd suggest dropping it to keep things simple.
| if [ "${install_post_arg}" -eq 1 ]; then | ||
| OPTIONS='--reinstall-packages-from= --skip-default-packages' | ||
| else | ||
| OPTIONS='-s -b -w -j --reinstall-packages-from= --lts --lts= --skip-default-packages --latest-npm --no-progress --alias= --default --save' |
There was a problem hiding this comment.
Missing --offline flag (and likely others as new flags are added over time - see comment about staleness above).
| ;; | ||
| esac | ||
|
|
||
| case "${previous_word}" in |
There was a problem hiding this comment.
This case "${previous_word}" block is now unreachable - the new case "${subcommand}" block above (lines 304-327) handles all the same subcommands and returns early. This dead code should be removed.
3 participants
Make option suggestions context-aware by subcommand and argument position to avoid invalid combinations.
Avoid stale or duplicate suggestions (including value flags and glob-like aliases) while preserving current-word completion.
Align option lists with nvm --help and observed CLI behavior for run/exec/ls/uninstall.
GitHub Copilot PR Summary: